Refactor septidon for the generic on field#19
Conversation
ControlCplusControlV
left a comment
ControlCplusControlV
left a comment
There was a problem hiding this comment.
Hello, I just had a couple of questions as someone reading through scroll repos :), if you don't mind answering it would be appreciated but also understand if you'd rather not have the PR clutter and can stop
naure
left a comment
naure
left a comment
There was a problem hiding this comment.
Great improvement! See comments.
| /// This implementation supports only the scalar field of BN254 at the moment. | ||
| /// | ||
| /// To implement for the Pasta curves, adjust the parameters below, and replace the transition round | ||
| /// by a copy, to get 56 rounds instead of 57. |
There was a problem hiding this comment.
This circuit does implement exactly 57 partial rounds, which is why it is not generic over any field. There should be at least an assert somewhere to check this. Maybe repeat this comment too in case someone wants to implement 56 rounds.
| pub mod sbox { | ||
| use super::super::util::pow_5; | ||
| use super::F; | ||
| use halo2_proofs::arithmetic::FieldExt; |
There was a problem hiding this comment.
The code does not compile at the moment.
We changed some things in halo2 recently to use the trait PrimeField instead of FieldExt, that might be the problem. See #13
There was a problem hiding this comment.
That is cause by the halo2 lib depency: the scroll-dev-1220 use another poseidon lib without a tag so it points to some unstable code and issue raised after several updating in the codebase.
I had update halo2 to scroll-dev-0220 (which is consistent with current zkevm-circuit) and the issue has been resolved.
This is no different than the existing traits ( It’s just that these traits are very complicated, which is why I chose initially the simpler way with |
Great, we could purpose another refactor / simplification later |
3 participants
Current the code (
SpongeChip) under "short" feature (i.e. septidon is applied) can not be wrapped into mpt and zkevm circuits for lacking of the generic on field. Notice the external caller consider poseidon circuit is generic by field withHashabletrait as constraints, we can not specializate it to bn254's field type too early.Here we have launched a wide range refactoring with noticing the original
Hashabletrait is not enough for sepctidon, since it require a 'cached' permutation constants being provided. We have to switch theHashabletrait by different feature (from the originalP128pow5t3to extendedCachedConstant), and update most of the septidon code with a field generic.